feat: implement updated design for regional access boundary#16084
feat: implement updated design for regional access boundary#16084nbayati wants to merge 7 commits intogoogleapis:mainfrom
Conversation
Make the fetching async and non blocking. Implement proactive refresh every 6 hours. Centralize the logic in a new class. Remove no-op signal and checks. Refactor to Regional Access Boundary name.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly overhauls the internal handling of regional access boundaries within the authentication library. The primary goal is to enhance the reliability and performance of this feature by introducing an asynchronous, non-blocking data fetching mechanism with proactive background refreshes. This refactoring centralizes the complex logic, streamlines error handling, and sets the stage for more robust regional access control. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the regional access boundary feature. The changes implement a more robust design with non-blocking, asynchronous fetching, proactive refresh, and exponential backoff on failure. The logic has been effectively centralized into new utility classes, and the feature has been consistently renamed from "Trust Boundary" to "Regional Access Boundary" across the codebase. The implementation appears solid and greatly improves the feature's resilience. I have a couple of minor suggestions to modernize the Python syntax for super() calls in the new and modified code.
packages/google-auth/google/auth/_regional_access_boundary_utils.py
Outdated
Show resolved
Hide resolved
|
|
||
| # For backwards compatibility with the previus feature name. | ||
| # Use CredentialsWithRegionalAccessBoundary instead. | ||
| CredentialsWithTrustBoundary = CredentialsWithRegionalAccessBoundary |
There was a problem hiding this comment.
Do you think we can keep the old code around with a deprecation note in the docstring? These changes make me a bit nervous.
I'm not sure if we'd expect anyone to subclass these classes, but if they do, they'd see a crash after this change, because some of the @abc.abstractmethods were changed, for example
If we can get away with implementing CredentialsWithRegionalAccessBoundary as a fresh class and leaving the old one alone, that could be safer
There was a problem hiding this comment.
Good point, I definitely agree we want to avoid crashing any external subclasses.
However, fully restoring the original CredentialsWithTrustBoundary class has its own set of issues. The old design had the trust boundary (now regional access boundary) fetching tied to the token refresh, and it used a blocking synchronous call every hour. We were through multiple design updates with the backend team, and the new design is async, non-blocking, and fail-open. It's much safer and seamless for the users. It also fetches the new RAB data every 6 hours as oppose to 1 hour, and has been decoupled from the token refresh flow. We don't expect customers to subclass these, but If we fully restore the old CredentialsWithTrustBoundary class alongside the new one, any customer who happened to subclass it will still be running the old design which is not favorable.
What if we keep CredentialsWithTrustBoundary and make it inherit from the new CredentialsWithRegionalAccessBoundary. It would look something like this:
class CredentialsWithTrustBoundary(CredentialsWithRegionalAccessBoundary):
"""
.. deprecated::
This class is deprecated. Please use CredentialsWithRegionalAccessBoundary instead.
"""
@abc.abstractmethod
def _build_trust_boundary_lookup_url(self):
"""Deprecated: Implement _build_regional_access_boundary_lookup_url instead."""
raise NotImplementedError()
def _build_regional_access_boundary_lookup_url(self, request=None):
warnings.warn(
"CredentialsWithTrustBoundary is deprecated. Use CredentialsWithRegionalAccessBoundary.",
DeprecationWarning,
)
return self._build_trust_boundary_lookup_url()
I think this way it satisfies the old abstract method contract, so existing customers' custom subclasses won't crash when they upgrade, without us having to restore the full CredentialsWithTrustBoundary class with the old logic. What do you think?
| "The trust_boundary parameter is deprecated and has no effect.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
We should also add this note to the Args: section in the docstrings
There was a problem hiding this comment.
The _trust_boundary field was added to the base credential class about 2 years ago, and it had stored a dummy value. It wasn't wired to the trust boundary logic we introduced last year, since the changes we made were limited to CredentialsWithTrustBoundary class, and weren't added to the base class.
I thought I could clean up the implementation as part of this PR, but I've realized that changing how _trust_boundary was stored, copied, and passed along could potentially break a unit test in one of the other libraries since this field has been around for 2 years.
I've also decided not to emit a deprecation warning since I'm keeping the behavior the same as before, storing and passing this value without any changes, this way we won't get repetitive deprecation warnings when the credential is copied and re-passed.
| return cred | ||
|
|
||
| @_helpers.copy_docstring(credentials.CredentialsWithTrustBoundary) | ||
| def with_trust_boundary(self, trust_boundary): |
There was a problem hiding this comment.
this seems like a public method we can't safely remove
There was a problem hiding this comment.
It's actually still available! The subclass now inherits with_trust_boundary directly from CredentialsWithRegionalAccessBoundary, so the public API is identical and we won't break any external callers. I just removed the redundant override to clean up the subclass.
| return cred | ||
|
|
||
| @_helpers.copy_docstring(credentials.CredentialsWithTrustBoundary) | ||
| def with_trust_boundary(self, trust_boundary): |
There was a problem hiding this comment.
this looks like another public one we can't drop
There was a problem hiding this comment.
Same for here, it inherits with_trust_boundary directly from CredentialsWithRegionalAccessBoundary.
| Manual Regional Access Boundary overrides are not supported. | ||
| This method is maintained for backwards compatibility and | ||
| returns a copy of the credentials without modifying the | ||
| Regional Access Boundary state. |
There was a problem hiding this comment.
Is this safe to do? I'm not too familiar with the security details here, but I could see it being dangerous to drop security configs like this after updating the library
There was a problem hiding this comment.
Would it be at all possible to keep the trust boundary logic in place, but just de-emphasize it in the docstrings? So existing code works as before?
There was a problem hiding this comment.
The trust boundary is currently no-op because the endpoint is returning a hardcoded dummy value, and the feature is behind an env variable.
The trust boundary (now called regional access boundary) acts as a routing hint for GFE, and doesn't enforce any security itself. If trust boundary is not provided, GFE will route the request to a set of mandatory region, which might be sub-optimal. The project hasn't been officially launched yet, and I'd be shocked if anyone is using this method, or the class in general. I believe GFE will ignore the headers for now, till we officially launch.
The reason we decided to scope out manual override for now, is that if the value is wrong or outdated, the request might get routed to a region where it doesn't have access, so the request ends up being rejected. Having no header is safer than having the wrong header, because no header means it'll still get routed to an appropriate region.
We are probably going to re-introduce the manual override in a future release with the new name (regional access boundary), and have it coupled with an auto recovery mechanism that can detect if the request was failed due to stale RAB, drop the header, and retry the request.
| from google.auth.transport import requests as google_auth_requests | ||
|
|
||
| request = google_auth_requests.Request() | ||
| request = google_auth_requests.Request() |
There was a problem hiding this comment.
It looks like requests is an optional dependency, so this will crash with an ImportError if it's not installed.
- Is that safe?
- Do we warn the user anywhere that requests is required for this feature?
- Are regional boundaries an opt-in feature, or is this auto-enabled in some environments?
There was a problem hiding this comment.
Can we use _http_client.Request() here, since that doesn't need any extra dependencies?
There was a problem hiding this comment.
I'm considering adding requests as a required dependency too, since it seems to be needed for mtls
There was a problem hiding this comment.
Great suggestion, done!
…gional_access_boundary_lookup_url` and add a new test.
….Request` for metadata server requests.
…ve type hinting across various credential types.
2 participants
Original PR: googleapis/google-auth-library-python#1955
Make the fetching async and non blocking.
Implement proactive refresh every 6 hours.
Centralize the logic in a new class.
Remove no-op signal and checks.
Refactor to Regional Access Boundary name.